Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for organization API #265

Merged
merged 12 commits into from
Nov 11, 2024
Merged

Add support for organization API #265

merged 12 commits into from
Nov 11, 2024

Conversation

sharifhsn
Copy link
Contributor

@sharifhsn sharifhsn commented Sep 3, 2024

Fixes #260

This PR currently only implements the invites module of the organization API. I'd like to get clarification on some details before I continue.

  • The organization API requires a different API key, denoted as OPENAI_ADMIN_KEY in documentation. I'm thinking that I should write an additional method on OpenAIConfig to use this environment variable, perhaps with_admin_key. What do you think?
  • All of the organization APIs are organized under that endpoint. As with the assistants module, I'm choosing to keep them in the flat namespace.
  • I'm choosing to name the types based on their names in the OpenAPI schema. Is this correct, or should I follow a different schema?
  • What is the minimum supported Rust version of this project? After checking with cargo-msrv, the current MSRV is 1.70.0. If this is intended, it should be indicated somewhere in the README.

@sharifhsn sharifhsn marked this pull request as draft September 3, 2024 15:32
@sharifhsn sharifhsn marked this pull request as ready for review September 3, 2024 15:32
@64bit
Copy link
Owner

64bit commented Sep 4, 2024

Fixes #260

This PR currently only implements the invites module of the organization API. I'd like to get clarification on some details before I continue.

Awesome! thank you for the PR.

* The organization API requires a different API key, denoted as `OPENAI_ADMIN_KEY` in documentation. I'm thinking that I should write an additional method on `OpenAIConfig` to use this environment variable, perhaps `with_admin_key`. What do you think?

Looks like admin API key is also a bearer token, then existing with_api_key is sufficient because user can decide to provide the right key.

Current behavior to read OPENAI_API_KEY env var on Client::new() can be extended to read OPENAI_ADMIN_KEY when non-admin key is not present.

* All of the `organization` APIs are organized under that endpoint. As with the `assistants` module, I'm choosing to keep them in the flat namespace.

Sounds good. I see the same top level grouping here https://platform.openai.com/docs/api-reference/administration

* I'm choosing to name the types based on their names in the OpenAPI schema. Is this correct, or should I follow a different schema?

Yes having 1-on-1 naming from spec to Rust types helps with single source of truth and on going maintenance. There are nested objects in spec which don't have name i usually go with <parent-schema-name><field-name> pattern for naming.

* What is the minimum supported Rust version of this project? After checking with `cargo-msrv`, the current MSRV is 1.70.0. If this is intended, it should be indicated somewhere in the README.

Isn't rust-version in Cargo.toml doing that? https://github.com/64bit/async-openai/blob/main/async-openai/Cargo.toml#L9
Is cargo-msrv a different thing?


Please consider adding a self contained example for this - it helps me test (& maintain) that serialization and de serialization of new types are working well.

Copy link
Owner

@64bit 64bit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this PR still needs work, and I have some bandwidth to make progress on it.
Hence I'm merging this, thank you for taking initiative on this one!

@64bit 64bit merged commit ac204a3 into 64bit:main Nov 11, 2024
@sharifhsn
Copy link
Contributor Author

Thanks, I got caught up in my work and open source fell by the wayside 😅 I appreciate the completion and merge!

@sharifhsn sharifhsn deleted the administration branch November 11, 2024 21:25
@64bit
Copy link
Owner

64bit commented Nov 12, 2024

😄 That happens right. You did a lot of heavy lifting in this one so I was able to pick up the remaining APIs which were not implemented for so long.

ifsheldon pushed a commit to ifsheldon/async-openai-wasm that referenced this pull request Nov 23, 2024
* Implement `invites`

* Consistently inline format

* Implement `users`

* Replace `InviteRole` with generic `OrganizationRole`

* Implement `projects`

* Update requests to use builder

* Remove `default` from builder

* Update `projects` to include verification information (https://help.openai.com/en/articles/9824607-api-platform-verifications)

* Implement `project_users`

---------

Co-authored-by: Himanshu Neema <[email protected]>

(cherry picked from commit ac204a3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for organization | administration APIs | sync spec
2 participants